Summary

This PR delivers its stated goal — trusted-server-core is now fully fastly-free and the consent pipeline is abstracted behind a minimal ConsentKvOps trait. Local verification passes (fmt, clippy -D warnings, cargo test --workspace, wasm32-wasip1 release build).

Three concerns merit changes before merge: a diagnostic regression in predict_backend_name_for_url, duplication of the security-adjacent SPOOFABLE_FORWARDED_HEADERS list, and a test-coverage regression in the relocated adapter compat.rs. The remaining items are non-blocking observations and follow-up candidates.

Note: this PR targets feature/edgezero-pr14-entry-point-dual-path, not main; review scope is the 31-file PR-15-specific delta (+2230/-2165).

Blocking

🔧 wrench

• Diagnostic regression in predict_backend_name_for_url — inline comment at crates/trusted-server-core/src/integrations/mod.rs:135
• SPOOFABLE_FORWARDED_HEADERS duplicated across core and adapter — inline comment at crates/trusted-server-adapter-fastly/src/compat.rs:18
• Adapter compat.rs has zero tests after relocation (14 security-relevant tests dropped) — inline comment at crates/trusted-server-adapter-fastly/src/compat.rs:82

Non-blocking

🤔 thinking

• KV errors logged at debug level as "lookup miss" — inline comment at crates/trusted-server-adapter-fastly/src/platform.rs:420
• Backend-name compute logic duplicated between predict_backend_name_for_url (core) and BackendConfig::compute_name (adapter) — inline comment at crates/trusted-server-core/src/integrations/mod.rs:130

🌱 seedling

• Redundant KV read on fallback+save path — inline comment at crates/trusted-server-core/src/consent/kv.rs:267
• Hardcoded certificate_check: true in ensure_integration_backend (integrations/mod.rs:69) and ensure_integration_backend_with_timeout (integrations/mod.rs:114). No regression vs. pre-PR behaviour, but the sibling predict_backend_name_for_url does take certificate_check: bool — asymmetric. If any integration ever needs a self-signed upstream (test/on-prem), these helpers would need to grow that parameter.

📌 out of scope

• migration_guards.rs not extended — migration_guards.rs:27-46 only covers 12 files and a narrow regex (Request|Response|http::Method|StatusCode|mime::APPLICATION_JSON). With core's fastly dep now removed, this guard should either broaden to \bfastly:: across all core files, be replaced by a Cargo-metadata dependency-presence test (more robust, no false negatives), or be deleted as structurally redundant. Follow-up issue.
• FastlyConsentKvStore::open silent failure — inline comment at crates/trusted-server-adapter-fastly/src/platform.rs:409

📝 note

• Stale "PR 15" forward references:
  • adapter-fastly/src/app.rs:150: "will be removed when legacy_main is deleted in PR 15". This is PR 15 and legacy_main was not deleted.
  • adapter-fastly/src/main.rs:113: "compat.rs will be deleted in PR 15 once this legacy path is retired". This is PR 15; compat.rs was moved (not deleted) and the legacy path was not retired.
    Update comments to reference the actual future PR number or drop the promise.

⛏ nitpick

• Unnecessary u16 type suffix — inline comment at crates/trusted-server-core/src/integrations/mod.rs:140

CI Status

• fmt: PASS
• clippy (-D warnings): PASS
• rust tests (cargo test --workspace): PASS (800+ tests)
• wasm32-wasip1 release build: PASS
• PR-reported checks (browser integration, integration tests, prepare integration artifacts): PASS

Summary

• Submitted reviewer-approved findings for consent/KV handling, backend naming consistency, and Fastly wiring.
• Verdict: REQUEST_CHANGES (includes a P1 finding).

Findings moved to review body

The following approved findings were included in the review body because GitHub could not resolve their requested inline locations in the current diff:

• 🤔 Consent bootstrap logic is now duplicated across auction and publisher paths (crates/trusted-server-core/src/auction/endpoints.rs:61)
  Both handlers now repeat the same sequence: parse cookies, generate/read synthetic ID, geo lookup with identical error handling, and build ConsentPipelineInput with kv_ops. This duplication is likely to drift as the consent pipeline evolves; this PR already had to touch both call sites just to thread the new abstraction through.
  Suggestion: Extract a small shared helper that prepares request-scoped consent state once and returns the ConsentContext plus any reused intermediates such as synthetic_id, geo, and cookie_jar.

• ⛏ Public docs/comments still lag the new kv_ops contract (crates/trusted-server-core/src/auction/endpoints.rs:21)
  The new kv_ops parameter materially changes how consent persistence is enabled, but the public docs still describe these handlers mostly in their pre-PR form, and one consent comment still says fallback requires consent_store rather than kv_ops.
  Suggestion: Update the affected doc comments to explain that consent KV fallback/write-through occurs only when a concrete kv_ops implementation is supplied, and adjust the stale internal comment in consent/mod.rs.

Summary

Approving this PR. The main refactor looks solid: trusted-server-core is decoupled from Fastly and the adapter now owns the Fastly-specific backend, KV, and compatibility wiring.

I left 2 non-blocking follow-up comments inline for logging/test coverage.

Summary

Re-review of PR 15 after the most recent commits (Apr 27). The core refactor goal is achieved — trusted-server-core no longer imports fastly, and the migration guard test (migration_guards.rs) durably enforces the invariant. Build, clippy, fmt, and the full Rust test suite (878 tests) pass locally; GitHub CI is green.

However, the merge with PR14 removed the ConsentKvOps / FastlyConsentKvStore abstraction without replacing the wiring on the EdgeZero entry path, leaving a privacy-sensitive gap: when edgezero_enabled = true and consent_store is configured, consent revocation deletes silently fail. The PR description still describes the pre-merge design and is now misleading.

Requesting changes to address (or explicitly accept and document) the EdgeZero consent-KV gap and to update the PR description before merge.

Blocking

❓ question

Consent KV is not opened on the EdgeZero entry path — crates/trusted-server-adapter-fastly/src/app.rs:92

build_state() constructs kv_store = Arc::new(UnavailableKvStore) unconditionally, and build_per_request_services (lines 111–127) hands that same store to every handler. The runtime_services_for_consent_route helper in crates/trusted-server-adapter-fastly/src/main.rs:307-323 (which actually opens the configured consent.consent_store) is only called from the legacy route_request.

Consequence: when edgezero_enabled = true and [consent] consent_store = "..." is configured, every services.kv_store() call in core returns KvError::Unavailable. In particular, the revocation delete in crates/trusted-server-core/src/publisher.rs:765 silently fails (Failed to delete consent KV entry … Unavailable) — the EC cookie expires, but the persisted consent data is not removed. KV-backed consent fallback also never engages on the EdgeZero path. This is a privacy/compliance concern (right-to-erasure).

This appears to be a regression from the merge resolution that removed FastlyConsentKvStore / open_consent_kv after merging PR14. Either (a) the EdgeZero path needs an equivalent of runtime_services_for_consent_route (open the consent store inside build_per_request_services or wrap the auction/publisher handlers), or (b) the EdgeZero path should fail loudly at startup when consent.consent_store is configured but cannot be opened.

Was this intentional, or did it slip through the merge?

🔧 wrench

PR description is stale and contradicts the merged code — The body still claims:

• "Introduces a sync ConsentKvOps trait in core"
• "FastlyConsentKvStore in the adapter implements the trait"
• "Threads kv_ops: Option<&dyn ConsentKvOps> through ConsentPipelineInput"

None of these exist after the PR14 merge — the actual code uses PlatformKvStore directly via RuntimeServices, and the consent KV is opened only on the legacy path. The "Changes" table for consent/kv.rs, consent/mod.rs, app.rs, main.rs, and platform.rs describes work that was reverted. Reviewers and git log consumers will be misled. Please update the PR description to reflect the post-PR14-merge reality before merging.

Non-blocking

🤔 thinking

UnavailableKvStore swallows configuration errors silently on the EdgeZero path — Combined with the question above, configuring consent_store = "missing-store" on the EdgeZero path produces no startup error and only per-request warn! lines. The legacy path correctly fails the auction/publisher routes with 503 (verified by route_tests.rs:184). Worth validating consent-store availability at startup so misconfiguration cannot silently disable revocation.

⛏ nitpick

Pre-existing: core is on edition = "2021" — CLAUDE.md mandates 2024 edition. Not introduced by this PR; flagged only for awareness/follow-up.

CI Status

• fmt: PASS (local + GitHub)
• clippy: PASS (local, -D warnings)
• rust tests: PASS (878 tests, local)
• GitHub CI: PASS (browser tests, integration tests, prepare integration artifacts)

Summary

Re-review of PR 15 against HEAD 725ccdc2. Local verification passes (fmt, clippy -D warnings, 878 + 45 unit tests across the changed crates) and GitHub CI is green.

The core refactor goal is achieved — trusted-server-core no longer imports fastly and the migration guard test enforces the invariant. However, the previously flagged EdgeZero consent-KV regression (Apr 30 review) is still present at the same line, and the PR description still describes the pre-PR14-merge design that was reverted. Requesting changes to address (or explicitly accept and document) those before merge.

Blocking

🔧 wrench

• EdgeZero path silently disables consent KV (privacy/compliance regression) — crates/trusted-server-adapter-fastly/src/app.rs:92 (line outside this PR's diff hunk, so flagging at body level).

  build_state() hardcodes Arc::new(UnavailableKvStore), and build_per_request_services (lines 111–127) hands that exact store to every route. There is no analogue of runtime_services_for_consent_route (main.rs:307-323) on this path.

  Concrete effect when edgezero_enabled = true AND [consent] consent_store = "..." is set:

  • crates/trusted-server-core/src/publisher.rs:765 calls delete_consent_from_kv(services.kv_store(), cookie_ec_id), which hits UnavailableKvStore.delete() → KvError::Unavailable. storage/kv_store.rs:323-332 swallows the error (warn-log only). The EC cookie expires but the persisted consent is not removed — right-to-erasure broken.
  • crates/trusted-server-core/src/auction/endpoints.rs:83-87 and publisher.rs:508-512 pass Some(services.kv_store()) to the consent pipeline, but it's the unavailable store, so consent persistence and KV-backed consent fallback never engage.

  Compare with the legacy path (correct): route_tests.rs:184-260 verifies that a missing consent.consent_store produces a fail-closed 503 on /auction and the publisher fallback via route_request. EdgeZero has no equivalent and silently degrades. Same blocking finding as the Apr 30 review, at the same line, still present at HEAD 725ccdc2.

  Fix options:

  1. Open consent.consent_store inside build_state() (or in build_per_request_services) using platform::open_kv_store, and either (a) fail startup if configured-but-unavailable or (b) wrap the consent-touching handlers (/auction, fallback) so they return 503 like the legacy path.
  2. Mirror runtime_services_for_consent_route at the EdgeZero handler boundaries.

  Either way, please add a counterpart of the existing configured_missing_consent_store_only_breaks_consent_routes test that drives TrustedServerApp::routes() so the regression cannot slip through another merge.

• PR description is stale and contradicts the merged code — the body still claims the PR introduces a sync ConsentKvOps trait, a FastlyConsentKvStore adapter implementation, and threads kv_ops: Option<&dyn ConsentKvOps> through ConsentPipelineInput. None of those exist after the PR14 merge — the actual code uses PlatformKvStore directly via RuntimeServices. The "Changes" table for consent/kv.rs, consent/mod.rs, app.rs, main.rs, and platform.rs describes work that was reverted. Please update the description to reflect the post-PR14-merge reality before merging so reviewers and git log consumers aren't misled.

Non-blocking

🤔 thinking

• No EdgeZero-side test for consent-store wiring — route_tests.rs:184 configured_missing_consent_store_only_breaks_consent_routes only exercises the legacy route_request. There is no analogous test that constructs TrustedServerApp::routes() with consent.consent_store configured. Combined with the blocking finding above, the regression slipped through the PR14 merge with no failing test to catch it.
• Both crates still on edition = "2021" — CLAUDE.md mandates 2024 edition. Pre-existing across the repo, not introduced by this PR; flagged for awareness/follow-up.

📌 out of scope

• UnavailableKvStore makes consent-store misconfiguration invisible at startup — even after the blocking finding is addressed, configured-but-unopenable stores will only surface as per-request warn! lines unless build_state() validates store availability at startup. Worth a follow-up to fail loudly when consent.consent_store is set but cannot be opened.

CI Status

• fmt: PASS (local)
• clippy: PASS (local, -D warnings)
• rust tests: PASS (878 + 45 in changed crates)
• GitHub CI: PASS (browser tests, integration tests, prepare integration artifacts)

Summary

This PR cleanly removes the fastly crate dependency from trusted-server-core, relocates the compat/backend layer into the adapter, migrates four integrations (aps, prebid, adserver_mock, sourcepoint) off BackendConfig to the platform-trait helpers, and restores the fail-closed consent KV wiring on the EdgeZero path for /auction and publisher fallback only. CI is green and local fmt/clippy/cargo test --workspace/wasm32-wasip1 build all pass.

Requesting changes on the items below — none are correctness-breaking today, but two of them (the select empty-backend-name fallback and the predict/ensure parameter asymmetry) sit on the bid-correlation path and can silently misbehave if upstream conditions shift.

Blocking

🤔 thinking — empty backend name silently propagates from select

crates/trusted-server-adapter-fastly/src/platform.rs:300-307 — when fastly_resp.get_backend_name() returns None, the code logs a warning and falls back to "". The PlatformResponse then carries backend_name == "", and AuctionOrchestrator correlates responses by exact string match against provider.backend_name(...). A bid response with "" will silently fail correlation — only the warning log signals the issue to operators.

Consider returning PlatformError::HttpClient here instead, so the orchestrator gets an explicit error frame for that bidder rather than a phantom non-match:

let ready = match result {
    Ok(fastly_resp) => match fastly_resp.get_backend_name() {
        Some(backend_name) => fastly_response_to_platform(fastly_resp, backend_name.to_string()),
        None => Err(Report::new(PlatformError::HttpClient)
            .attach("select: response has no backend name; correlation impossible")),
    },
    Err(e) => Err(Report::new(PlatformError::HttpClient)
        .attach(format!("fastly select error: {e}"))),
};

(Section unchanged in this PR, so flagging in body rather than inline — fix in this PR or open a follow-up.)

🌱 seedling — no compile-time guard that services.kv_store() is the consent KV

crates/trusted-server-core/src/publisher.rs:764-766 — correctness relies on the caller having threaded runtime_services_for_consent_route(...) before handle_publisher_request. Today the wiring is right (legacy route_request and EdgeZero dispatch_fallback's publisher branch both do it), but there's no compile-time guard. If a future entry point forgets, services.kv_store() will be UnavailableKvStore and delete will succeed-as-noop, leaving stale consent KV entries after a user revokes.

A typed ConsentKvStore(Arc<dyn PlatformKvStore>) newtype constructed only by runtime_services_for_consent_route would make the wiring mistake a compile error. Worth doing while this surface is fresh.

Non-blocking

♻️ refactor — duplicate enable_ssl().sni_hostname(self.host) in BackendConfig::ensure

crates/trusted-server-adapter-fastly/src/backend.rs:177-188 — enable_ssl().sni_hostname(self.host) is called once unconditionally, then a second time inside the if self.certificate_check branch before .check_certificate(host). Idempotent on the builder, so functionally fine, but easy to simplify on the file's move into the adapter:

builder = builder.enable_ssl().sni_hostname(self.host);
if self.certificate_check {
    builder = builder.check_certificate(self.host);
} else {
    log::warn!(
        "INSECURE: certificate check disabled for backend: {}",
        backend_name
    );
}
log::info!("enable ssl for backend: {}", backend_name);

Pre-existing in the deleted core copy; the relocation is the natural moment to clean it up. (Section unchanged in the rename diff, so flagging in body rather than inline.)

📝 note — AppState.kv_store is now always UnavailableKvStore

crates/trusted-server-adapter-fastly/src/app.rs:103 and the matching build_per_request_services site — per-route consent stores are opened on demand via runtime_services_for_consent_route, and no other consumer reads services.kv_store(). The field is now a placeholder, but its name reads as if it were the primary KV. Consider either inlining Arc::new(UnavailableKvStore) at the two construction sites, or renaming the field to default_kv_store to signal that it's only the fallback when no consent route override applies.

📝 note — three AuctionProvider::backend_name impls replicate identical bodies

aps.rs, prebid.rs, adserver_mock.rs all paste essentially the same body wrapping predict_integration_backend_name(&self.config.{endpoint,server_url}, …). A default method on AuctionProvider parameterised by (endpoint: &str, integration_id: &'static str) would remove the boilerplate; doing so would also make the dead certificate_check parameter (see the inline ♻️ refactor on integrations/mod.rs) impossible to misuse.

⛏ nitpick — from_fastly_request panics on URL parse failure

crates/trusted-server-adapter-fastly/src/compat.rs:11-28 — the deleted crates/trusted-server-core/src/compat.rs parsed the URL with a / fallback and warning log; this version .expects. Fastly delivers validated URLs in practice, and the # Panics doc is honest, so this is defensible — but the previous "log + continue" was strictly more defensive on a path that processes every inbound request. Preference call.

CI Status

• fmt: PASS
• clippy: PASS (no warnings)
• rust tests: PASS (950 + 54 + 21 + 3, doctests included)
• wasm32-wasip1 release build: PASS
• GitHub Actions: PASS (browser-integration, integration, prepare-integration-artifacts)
• migration_guards.rs invariant (no fastly::Request/Response/... in migrated core modules): PASS
• Verified zero use ... fastly imports remain anywhere under crates/trusted-server-core/src/

Summary

Re-review of PR 15 (remove fastly from trusted-server-core), reviewed against its actual base feature/edgezero-pr14-entry-point-dual-path to isolate this PR's 32-file changeset. Round-1 findings are addressed (select() None→Err, predict/ensure unified via integration_backend_spec, integration-route consent-bypass test, default_kv_store rename). One requested change remains: the tokio test-migration is half-done and leaves the codebase with two inconsistent async-test conventions. The remaining items are non-blocking.

Blocking

🔧 wrench

• Incomplete tokio removal: GTM tests converted to block_on (~672 lines) but proxy.rs still uses #[tokio::test] and tokio remains a dev-dependency — finish the conversion + drop the dep, or revert the churn. (Cargo.toml:79, google_tag_manager.rs, proxy.rs:1520)

Non-blocking

🤔 thinking

• from_fastly_request now panics on unparseable URL: changed from "/" fallback to .expect(); confirm intentional. (adapter/compat.rs:15)

🌱 seedling

• Speculative getrandom/uuid js deps for wasm32-unknown-unknown, a target no CI job builds. (core/Cargo.toml:56)

⛏ nitpick

• Stale fastly:: doc-comment references missed by the cleanup commit: proxy.rs:1485, proxy.rs:2386 (fastly::Response), platform/mod.rs:28 (fastly::Body). streaming_processor.rs was updated to "any platform body type"; these were missed.

CI Status

The PR's GitHub CI ran only the 3 integration-test jobs (it targets a feature branch, so the standard gates didn't trigger). Verified locally against the PR head:

• fmt: PASS
• clippy (--all-targets --all-features -D warnings): PASS
• rust tests: PASS (core 950, adapter 54)
• wasm32-wasip1 release build: PASS

Summary

Re-review of PR 15 against HEAD cedea8b1, diffed against its actual base feature/edgezero-pr14-entry-point-dual-path. The previously blocking item — incomplete tokio removal — is resolved: 0 #[tokio::test] remain, tokio is gone from core's manifest, and the GTM conversion is lossless (9→9 tests, 14→14 assertions, 10→10 awaits). Stale fastly:: doc references are fixed.

This pass surfaced four new issues, two blocking. The core refactor mechanics (compat/backend relocation, platform-abstraction threading, consent-route fail-closed 503 wiring, deterministic backend naming) are sound and well-covered — those are not repeated here.

Blocking

🔧 wrench

• from_fastly_request panics on an unparseable URL — regressed from a graceful "/" fallback to .expect(), on the default legacy path. (adapter/compat.rs:15)
• Duplicate, IPv6-divergent EC-ID generator — edge_cookie.rs now carries its own HMAC generator whose /64 normalization disagrees with canonical ec::generation::normalize_ip; same passphrase ⇒ IPv4 IDs match but IPv6 IDs diverge, minting non-correlating identities. (edge_cookie.rs:39 vs ec/generation.rs:35)
• Consent-KV read-fallback / write-on-change advertised but unwired — ConsentPipelineInput.{ec_id,kv_store} and load_consent_from_kv/save_consent_to_kv are dead in production; build_consent_context ignores the fields. (consent/mod.rs:82, storage/kv_store.rs)

❓ question

• PR goal unmet: trusted-server-core still depends on fastly — core/Cargo.toml:25 still declares it, kept alive by ec/kv.rs (fastly::kv_store) and ec/rate_limiter.rs (fastly::erl). The changes-table claim that core's fastly dependency is removed is currently false — is the EC KV/ERL migration deferred?

Non-blocking

🏕 camp site

• Orphaned crates/trusted-server-core/src/backend.rs — lib.rs dropped pub mod backend;, but the ~16 KB file is still tracked, no longer compiled, and still carries use fastly::backend::Backend. Should be git rm'd alongside the lib.rs change.

📝 note

• Duplicated DEFAULT_FIRST_BYTE_TIMEOUT — added at platform/mod.rs:58 while an identical pub(crate) const remains in the orphaned backend.rs:37. Collapses to one definition once the dead file above is removed.
• Removed lgtm[rust/cleartext-logging] suppressions in publisher.rs — if CodeQL/lgtm still runs in CI, confirm dropping these doesn't re-introduce cleartext-logging alerts on the affected debug logs.

CI Status

The PR's GitHub CI ran only the 3 integration-test jobs (it targets a feature branch, so the standard fmt/clippy/test gates didn't trigger). All 3 PASS.

• integration tests: PASS
• browser integration tests: PASS
• prepare integration artifacts: PASS

Summary (re-review)

The three prior-round findings are resolved: tokio is fully removed from core (all #[tokio::test] converted to block_on, dep dropped), the stale fastly:: doc comments are gone, and from_fastly_request reverted to the graceful "/" fallback. All local gates pass (see CI Status).

However, a merge of the latest PR14 (EC-identity port) reintroduced a hard fastly dependency in core (711dd6cb), so the PR no longer matches its own title/description. That, plus a guard that no longer guards and a now-unreachable target block, are the requested changes. The underlying engineering deferral is defensible — the asks are about accuracy and consistency, not the code's behavior.

Blocking

🔧 wrench

• PR no longer removes fastly from core; Closes #496 is inaccurate. ec/kv.rs (fastly::kv_store::{InsertMode, KVStore} + KVStoreError) and ec/rate_limiter.rs (fastly::erl) still import the Fastly SDK, and Cargo.toml re-adds fastly unconditionally. Issue #496's Done when is "Core has zero fastly imports." Either abstract those two EC usages behind platform traits, or correct the title/body and drop Closes #496 (rescope the residual under #495, which the in-code comment already references). (core/Cargo.toml:29, ec/kv.rs:14, ec/rate_limiter.rs:8)
• migration_guards.rs gives false assurance. The PR body says the no-fastly invariant is "enforc[ed] with migration_guards.rs," but its regex only bans fastly::(Request|Response|http::Method|StatusCode|mime) and scans a hardcoded sources list that excludes ec/kv.rs and ec/rate_limiter.rs. It neither catches the reintroduced usage nor enforces "no fastly." Broaden it (with an explicit allowlist for the deferred EC files) or soften the claim. (migration_guards.rs:27, migration_guards.rs:48)

Non-blocking

🤔 thinking

• The wasm32-unknown-unknown js dependency block is now unreachable. Core has an unconditional fastly dep (fastly only builds for wasm32-wasip1), so core can never compile for wasm32-unknown-unknown — the exact target the getrandom/uuid js block exists for. Gate fastly behind a target/feature cfg, or drop the speculative block until core can actually target Workers. (#496 lists wasm32-unknown-unknown as an intended core target, so this tension is in-scope.) (core/Cargo.toml:61)

📝 note

• Diff is inflated by a stale PR14 merge. The ec/*, proxy.rs, and publisher.rs deltas are largely PR14's EC port at an older revision than PR14's current tip, not PR15 changes. Re-merging the latest PR14 base would shrink the diff considerably and isolate this PR's actual contribution. Not a code defect.

CI Status

The PR's GitHub CI ran only the 3 integration-test jobs (it targets a feature branch). Verified locally against head 711dd6cb:

• fmt: PASS
• clippy (--all-targets --all-features -D warnings): PASS
• rust tests: PASS (core 1237, adapter 62)
• wasm32-wasip1 release build: PASS